Only create a single publish channel#1620
Merged
Merged
Conversation
f696571 to
50fb15c
Compare
Contributor
Author
|
I have also tested another implementation using a Lazy that roughly looks something like that public async ValueTask<ConfirmsAwareChannel> GetPublishChannel(CancellationToken cancellationToken = default)
{
var maybePublishChannel = publishChannel;
if (maybePublishChannel is not null)
{
var channel = await maybePublishChannel.Value.ConfigureAwait(false);
if (channel.IsOpen)
{
return channel;
}
Interlocked.CompareExchange(ref publishChannel, null, maybePublishChannel);
}
var maybeNewChannel = new Lazy<Task<ConfirmsAwareChannel>>(() => Task.Run(async () =>
{
var newChannel = new ConfirmsAwareChannel(connection!, routingTopology);
await newChannel.Initialize(cancellationToken).ConfigureAwait(false);
return newChannel;
}));
var actualLazy = Interlocked.CompareExchange(ref publishChannel, maybeNewChannel, null) ?? publishChannel!;
return await actualLazy.Value.ConfigureAwait(false);
}
public async ValueTask ReturnPublishChannel(ConfirmsAwareChannel channel, CancellationToken cancellationToken = default)
{
if (channel.IsOpen)
{
return;
}
var maybePublishChannel = publishChannel;
if (maybePublishChannel is not null)
{
var currentChannel = await maybePublishChannel.Value.ConfigureAwait(false);
if (ReferenceEquals(currentChannel, channel))
{
Interlocked.CompareExchange(ref publishChannel, null, maybePublishChannel);
await channel.DisposeAsync().ConfigureAwait(false);
}
}
}
#pragma warning disable PS0018
public async ValueTask DisposeAsync()
#pragma warning restore PS0018
{
if (disposed)
{
return;
}
await stoppingTokenSource.CancelAsync().ConfigureAwait(false);
stoppingTokenSource.Dispose();
var oldConnection = Interlocked.Exchange(ref connection, null);
oldConnection?.Dispose();
var maybeOldChannel = Interlocked.Exchange(ref publishChannel, null);
if (maybeOldChannel is { IsValueCreated: true })
{
var oldChannel = await maybeOldChannel.Value.ConfigureAwait(false);
await oldChannel.DisposeAsync().ConfigureAwait(false);
}
disposed = true;
}
readonly CancellationTokenSource stoppingTokenSource = new();
volatile IConnection? connection;
volatile Lazy<Task<ConfirmsAwareChannel>>? publishChannel;
bool disposed;but there is no additional benefit and the complexity is much higher. |
bording
reviewed
Jun 10, 2025
Co-authored-by: Brandon Ording <bording@gmail.com>
bording
approved these changes
Jun 10, 2025
danielmarbach
added a commit
that referenced
this pull request
Jun 11, 2025
* Use a single publish channel with atomic swaps * Cosmetics * Few tweaks * Simplify implementation * Remove null-forgiving Co-authored-by: Brandon Ording <bording@gmail.com> --------- Co-authored-by: Daniel Marbach <danielmarbach@users.noreply.github.com> Co-authored-by: Brandon Ording <bording@gmail.com>
danielmarbach
added a commit
that referenced
this pull request
Jun 11, 2025
* Use a single publish channel with atomic swaps * Cosmetics * Few tweaks * Simplify implementation * Remove null-forgiving Co-authored-by: Brandon Ording <bording@gmail.com> --------- Co-authored-by: Daniel Marbach <danielmarbach@users.noreply.github.com> Co-authored-by: Brandon Ording <bording@gmail.com>
danielmarbach
added a commit
that referenced
this pull request
Jun 11, 2025
* Use a single publish channel with atomic swaps * Cosmetics * Few tweaks * Simplify implementation * Remove null-forgiving --------- Co-authored-by: Daniel Marbach <danielmarbach@users.noreply.github.com> Co-authored-by: Brandon Ording <bording@gmail.com>
danielmarbach
added a commit
that referenced
this pull request
Jun 11, 2025
* Use a single publish channel with atomic swaps * Cosmetics * Few tweaks * Simplify implementation * Remove null-forgiving Co-authored-by: Brandon Ording <bording@gmail.com> --------- Co-authored-by: Daniel Marbach <danielmarbach@users.noreply.github.com> Co-authored-by: Brandon Ording <bording@gmail.com>
danielmarbach
added a commit
that referenced
this pull request
Jun 11, 2025
* Use a single publish channel with atomic swaps * Cosmetics * Few tweaks * Simplify implementation * Remove null-forgiving Co-authored-by: Brandon Ording <bording@gmail.com> --------- Co-authored-by: Daniel Marbach <danielmarbach@users.noreply.github.com> Co-authored-by: Brandon Ording <bording@gmail.com>
danielmarbach
added a commit
that referenced
this pull request
Jun 11, 2025
* Use a single publish channel with atomic swaps * Cosmetics * Few tweaks * Simplify implementation * Remove null-forgiving --------- Co-authored-by: Daniel Marbach <danielmarbach@users.noreply.github.com> Co-authored-by: Brandon Ording <bording@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

Fixes #1621
Historically the underlying SDK used was dispatching synchronously the network requests and our code added some additional asynchronous parts using a custom scheduler that waited for the publisher confirms to return. The previous guidance of the RabbitMQ client suggested channels should not be re-used across concurrent operations. That's why the channel provider code was added to make sure during concurrent operations clients are never concurrently accessed. Given the synchronous nature of the SDK calls it was rarely ever possible for more than one or two channels being managed by the channel provider.
With the new async nature of the SDK that also contains an implementation to manage publisher confirms asynchronously (contributed by us) the SDK calls are now completely asynchronous. During the migration to the new client, this detail was overlooked. This leads to massive concurrent access to the channel provider and the channel provider trying to create N number of channels which leads to slow down of publish operations and channels lingering around until the endpoint is restarted or in the worst case the client operations reaching the maximum number of channels allowed.
This PR changes the channel provider to lazy create a single publish channel (since channels are now thread safe as long as they are not used for publishing and consumptions at the same time). The channel provider still applies a rent and return pattern to make sure a faulty channel is closed when returned and new publish operations acquire a fresh channel when needed.
I have played around with multiple internal implementations using
AsyncManualResetEventor CompareExchange/ Spin but ultimately concluded the semaphore version is best given channels are expensive to create.Benchmark
Results